Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ci: increase page size of GH PR API call #13518

Merged
merged 1 commit into from
Mar 9, 2021
Merged

ci: increase page size of GH PR API call #13518

merged 1 commit into from
Mar 9, 2021

Conversation

villebro
Copy link
Member

@villebro villebro commented Mar 8, 2021

SUMMARY

When working on #13484 I noticed that the script that checks for python and frontend changes in the PR wasn't picking up changes to python files:

GITHUB_REPO=apache/superset PR_NUMBER=13484 ./scripts/ci_check_no_file_changes.sh python
Searching for changes in python files

CHANGED FILES:
superset-frontend/package-lock.json
superset-frontend/package.json
superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.tsx
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterValue.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts
superset-frontend/src/dashboard/components/nativeFilters/utils.ts
superset-frontend/src/dataMask/actions.ts
superset-frontend/src/filters/components/Range/buildQuery.ts
superset-frontend/src/filters/components/Select/buildQuery.ts
superset-frontend/src/filters/components/TimeColumn/TimeColumnFilterPlugin.tsx
superset-frontend/src/filters/components/TimeColumn/buildQuery.ts
superset-frontend/src/filters/components/TimeColumn/controlPanel.ts
superset-frontend/src/filters/components/TimeColumn/images/thumbnail.png
superset-frontend/src/filters/components/TimeColumn/index.ts
superset-frontend/src/filters/components/TimeColumn/transformProps.ts
superset-frontend/src/filters/components/TimeColumn/types.ts
superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx
superset-frontend/src/filters/components/TimeGrain/buildQuery.ts
superset-frontend/src/filters/components/TimeGrain/controlPanel.ts
superset-frontend/src/filters/components/TimeGrain/images/thumbnail.png
superset-frontend/src/filters/components/TimeGrain/index.ts
superset-frontend/src/filters/components/TimeGrain/transformProps.ts
superset-frontend/src/filters/components/TimeGrain/types.ts
superset-frontend/src/filters/components/index.ts
superset-frontend/src/filters/utils.ts
superset-frontend/src/visualizations/presets/MainPreset.js

No changes detected... Exiting with SUCCESS code

Turns out GH API calls are paginated. After adding the per_page query parameter the full set of changes were reported:

GITHUB_REPO=apache/superset PR_NUMBER=13484 ./scripts/ci_check_no_file_changes.sh python
Searching for changes in python files

CHANGED FILES:
superset-frontend/package-lock.json
superset-frontend/package.json
superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.tsx
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterValue.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts
superset-frontend/src/dashboard/components/nativeFilters/utils.ts
superset-frontend/src/dataMask/actions.ts
superset-frontend/src/filters/components/Range/buildQuery.ts
superset-frontend/src/filters/components/Select/buildQuery.ts
superset-frontend/src/filters/components/TimeColumn/TimeColumnFilterPlugin.tsx
superset-frontend/src/filters/components/TimeColumn/buildQuery.ts
superset-frontend/src/filters/components/TimeColumn/controlPanel.ts
superset-frontend/src/filters/components/TimeColumn/images/thumbnail.png
superset-frontend/src/filters/components/TimeColumn/index.ts
superset-frontend/src/filters/components/TimeColumn/transformProps.ts
superset-frontend/src/filters/components/TimeColumn/types.ts
superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx
superset-frontend/src/filters/components/TimeGrain/buildQuery.ts
superset-frontend/src/filters/components/TimeGrain/controlPanel.ts
superset-frontend/src/filters/components/TimeGrain/images/thumbnail.png
superset-frontend/src/filters/components/TimeGrain/index.ts
superset-frontend/src/filters/components/TimeGrain/transformProps.ts
superset-frontend/src/filters/components/TimeGrain/types.ts
superset-frontend/src/filters/components/index.ts
superset-frontend/src/filters/utils.ts
superset-frontend/src/visualizations/presets/MainPreset.js
superset/charts/schemas.py
superset/common/query_object.py
superset/connectors/druid/models.py
superset/connectors/sqla/models.py
superset/db_engine_specs/base.py
superset/utils/core.py
tests/core_tests.py
tests/dashboard_utils.py
tests/databases/commands_tests.py
tests/fixtures/birth_names_dashboard.py
tests/query_context_tests.py

Detected changes in following file: superset/charts/schemas.py
Exiting with FAILURE code

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@zhaoyongjie zhaoyongjie self-requested a review March 9, 2021 06:08
zhaoyongjie
zhaoyongjie approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
the default pagination for Github is 30.
https://docs.github.com/en/rest/overview/resources-in-the-rest-api#pagination

@villebro villebro merged commit 40fc144 into apache:master Mar 9, 2021
ktmud
ktmud reviewed Mar 9, 2021
View reviewed changes
scripts/ci_check_no_file_changes.sh
@@ -27,7 +27,7 @@ if [[ -z ${PR_NUMBER} ]]; then
exit 1
fi

URL="https://api.github.com/repos/${GITHUB_REPO}/pulls/${PR_NUMBER}/files"
URL="https://api.github.com/repos/${GITHUB_REPO}/pulls/${PR_NUMBER}/files?per_page=1000"
Copy link
Member

@ktmud ktmud Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I'm late to the party again. The maximum per_page size is 100 according to the document, although I'd assume we won't regularly exceed that limit anyway.

To obtain more than 100 items, there is a convenient pagination helper in GitHub script.
This PR has an example.

For convenience and future extensibility, we might want to convert this file to GitHub script the next time we touch it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually intending to make a cleaner solution, this is to unblock immediate PRs suffering from this. I'll fix this properly this week.

@villebro villebro deleted the villebro/ci_per_page branch March 9, 2021 07:18
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
@villebro
ci: increase page size of GH PR API call (apache#13518)
3e005db
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktmud ktmud ktmud left review comments

@zhaoyongjie zhaoyongjie zhaoyongjie approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io 🚢 1.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@villebro @ktmud @zhaoyongjie @mistercrunch